Skip to content

Added in copy button #1073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 9 commits into
base: gh-pages
Choose a base branch
from
Open

Added in copy button #1073

wants to merge 9 commits into from

Conversation

@Yash-Singh1
Copy link

@Yash-Singh1 Yash-Singh1 commented Aug 24, 2020 •
edited
Loading

Overview

Added in a button called Copy Result with an ID of copy-btn allowing you to copy the result of the minification process.

List of Updates

Here is a list of the updates:

  • Added in a button inside index.html with the same CSS as the earlier minify-btn
  • Enabled the copy-btn on click of the minify-btn
  • Copied the result on click of the copy-btn

Testing

To test my changes inside a development environment, go to: https://yash-singh1.github.io/html-minifier/

DanielRuf
DanielRuf reviewed Aug 26, 2020
View reviewed changes
DanielRuf
DanielRuf reviewed Aug 26, 2020
View reviewed changes
DanielRuf
DanielRuf reviewed Aug 26, 2020
View reviewed changes
@Yash-Singh1
Copy link
Author

Any updates on this pull request, or is it good to go?

@DanielRuf
Copy link

LGTM

@Yash-Singh1
Copy link
Author

So, will @kangax merge it?

@kangax
Copy link
Owner

kangax commented Sep 21, 2020

hm, Travis CI doesn't let me merge, says some checks haven't completed yet

@Yash-Singh1
Copy link
Author

Yash-Singh1 commented Sep 23, 2020 •
edited
Loading

@kangax Looks like it says that here too:
Screenshot from 2020-09-22 17-53-34
Look at this:
https://travis-ci.community/t/known-issue-travis-ci-reports-expected-waiting-for-status-to-be-reported-on-the-github-status-api-but-the-status-never-arrives/1154
I think it is a fix. Looks like this was an issue: travis-ci/travis-ci#10187
Here is another write-up someone wrote on the fix: travis-ci/travis-ci#10204
I think the solution is just to make sure that you have access to merge this pull request even when the tests haven't passed. This should be enabled in scenarios in which you want to force merge. I ran the tests manually, here is what appeared:
Screenshot from 2020-09-22 18-03-13
If you want a close up of the results:
Screenshot from 2020-09-22 18-03-54
Looks like all the tests worked manually. 👍

@Yash-Singh1
Copy link
Author

Since html-minifier now uses Gh Actions, can this be merged?

@kangax
Copy link
Owner

kangax commented Oct 8, 2021 •
edited
Loading

yeah, I'm ok with it

@XhmikosR any objections?

@XhmikosR
Copy link
Collaborator

XhmikosR commented Oct 8, 2021

Yeah, this shouldn't land as is. innerHTML is meaningless when we only need to change the text.

Also, I believe we should keep the initial text instead of re-adding it.

@DanielRuf
Copy link

Yeah, this shouldn't land as is. innerHTML is meaningless when we only need to change the text.

You are right, innerText should be sufficient then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@DanielRuf DanielRuf DanielRuf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Yash-Singh1 @DanielRuf @kangax @XhmikosR